Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deleting all files in trash now only sends a single flag #5641

Merged
merged 1 commit into from
Nov 25, 2013

Conversation

PVince81
Copy link
Contributor

To prevent having to send the list of all files for deletion, only set a flag "allfiles". This should make it a bit smoother when deleting 5000+ files.

Also fixes some "empty trash" message issues.

@PVince81
Copy link
Contributor Author

It's late... getting all the issues numbers wrong...

@PVince81
Copy link
Contributor Author

Setting to OC6 if this fix/workaround is acceptable.

@karlitschek
Copy link
Contributor

Great 👍

@ghost
Copy link

ghost commented Oct 30, 2013

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1865/

@schiessle
Copy link
Contributor

@PVince81 Thanks for taking the initiative here! But I would really like to hear @jancborchardt opinion.

Basically we now have 4 ways to remove files permanently from the trash bin:

  • Delete single files or a selection

(1) remove a single file by the delete button on the right side.
(2) select some files and click the delete button above the files list

  • Delete all files

(3) select all with the "group select" next to the "Name" column header and than click the delete button on the right side
(4) click the new introduced button directly

I'm not generally against it. But if I look at all the options to delete a file permanently it also looks a bit crowded.

As an alternative wouldn't it be possible to implement a simple infinite scroll just for the trash bin? For the trash bin we don't have all the corner cases we have for the regular files view. @PVince81 I know you already played around with it, what do you think?

@jancborchardt
Copy link
Member

Hm, I’m really hesitant as to pulling this in in the beta phase, especially
such a destructive feature.

Also, design-wise – we don’t really need »empty trash« at all and I think
we should not embrace that model of cleaning stuff out completely. The
trash is cleaned automatically when there’s not enough space left (right,
@schiesbn) and if you empty the trash, the whole history is gone in one
fell swoop.

So I’m against this. It’s not just a simple fix for helping people delete
everything, but a fundemantal decision on how we want to handle storage and
history.

On Thu, Oct 31, 2013 at 10:59 AM, Björn Schießle
[email protected]:

@PVince81 https://github.com/PVince81 Thanks for taking the initiative
here! But I would really like to hear @jancborchardthttps://github.com/jancborchardtopinion.

Basically we now have 4 ways to remove files permanently from the trash
bin:

  • Delete single files or a selection

(1) remove a single file by the delete button on the right side.
(2) select some files and click the delete button above the files list

  • Delete all files

(3) select all with the "group select" next to the "Name" column header
and than click the delete button on the right side
(4) click the new introduced button directly

I'm not generally against it. But if I look at all the options to delete a
file permanently it also looks a bit confusing.

As an alternative wouldn't it be possible to implement a simple infinite
scroll just for the trash bin? For the trash bin we don't have all the
corner cases we have for the regular files view. @PVince81https://github.com/PVince81I know you already played around with it, what do you think?


Reply to this email directly or view it on GitHubhttps://github.com//pull/5641#issuecomment-27473723
.

@PVince81
Copy link
Contributor Author

The problem with infinite scrolling is that it needs many changes in the way how files are returned, changes that are bigger than just that empty trash button (and some of them involve touching filelist.js from the files package).

If the trash is indeed automatically cleant like the version apps does then there is probably no need for this now and the two users with too many files should be able to not exceed their quotas.

@schiessle
Copy link
Contributor

Also, design-wise – we don’t really need »empty trash« at all and I think we should not embrace that model of cleaning stuff out completely. The trash is cleaned automatically when there’s not enough space left (right, @schiesbn) and if you empty the trash, the whole history is gone in one fell swoop.

Right. But there is one problem which we could solve with the button: If you have really a lot of files in one folder of the trash your web interface will become unresponsive.
As long as the user uses the web interface I don't think that this can happen. Nobody deletes thousands of single files without the trash bin cleans it up in between. But it can happen quite easily if people uses the sync client or any other webdav client. Because in this case ownCloud will receive one delete operation for every single file if you delete a folder. So if someone deletes a folder with many nested sub-folders and files the trash bin will show them as a flat file list directly in the root. Because for the trash bin it wasn't a folder delete but many single file deletes.
This is also bad in other aspects. It will not only generate a huge list of deleted files but will also add every sub folder as an empty deleted folder which makes it almost impossible to restore the old folder structure again. BTW, this should be fixed with sync client 1.5 which will send a single delete request for a folder. But the problem will probably still exists for other wbedav clients.

@PVince81
Copy link
Contributor Author

Now thinking of the infinite scroll, we might still need that backend part I implemented in this PR.
Because: if you click "select all" there won't be any DOM elements for the pages that haven't been rendered yet if the user doesn't scroll down. This means we can't send the whole file list as it's done on master.
So for all commands that support "select all" we'd need to add a "allfiles=true" URL argument.

@PVince81
Copy link
Contributor Author

So basically "select all" + "delete" would do the same under the hood as what my "empty trash" button now does.

@jancborchardt
Copy link
Member

Right, I was thinking that as well – the select all + delete action should
for many files just fake the selection of everything. It doesn’t need to
actually do it, but if you use select all and directly after that use the
delete action, it’s factually the same. (We didn’t have the delete multi
action before, and this »empty trash« workaround was one kind-of reason why
we added it.)

On Thu, Oct 31, 2013 at 11:38 AM, Vincent Petry [email protected]:

So basically "select all" + "delete" would do the same under the hood as
what my "empty trash" button now does.


Reply to this email directly or view it on GitHubhttps://github.com//pull/5641#issuecomment-27475929
.

@PVince81
Copy link
Contributor Author

Yes, now the trouble might be that clicking "Select all" might take ages with 12000 files as it needs to update all the DOM elements. Still, if the user has patience, deleting this way might work.

So, would this PR be acceptable if I use "deleteall=true" if the user selects everything ?

What if new files have appeared in the trashbin that weren't visible before ? (this is a general issue that we'll have later with infinite scrolling as well)

@schiessle
Copy link
Contributor

So, would this PR be acceptable if I use "deleteall=true" if the user selects everything ?

Together with an infinite scroll?

Yes, now the trouble might be that clicking "Select all" might take ages with 12000 files

with an infinite scroll we wouldn't need to select all 12000 files but only the visible ones so that it looks like everything is selected.

What if new files have appeared in the trashbin that weren't visible before ?

I think that's a real corner case that a user browses his trash bin and deletes some files at the same time at the desktop client. I think it is acceptable if this files doesn't show up before a reload. With respect to the "delete all" operation I would just delete all files which are in the trash bin when the operation gets triggered. Instead of a file list I would send the delete all flag to the ajax method which than cleans both the trash and the database.

@ghost ghost assigned PVince81 Nov 6, 2013
To prevent having to send the list of all files for deletion, only set a
flag "allfiles". This should make it a bit smoother when deleting 5000+
files.

Also fixes some "empty trash" message issues.
@PVince81
Copy link
Contributor Author

I've rebased this and changed the description.

Now it sends "allfiles=true" when deleting all files, which should make deleting a bit quicker.
It might still take some time to highlight all files when selecting.

@ghost
Copy link

ghost commented Nov 11, 2013

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1971/

@MorrisJobke
Copy link
Contributor

Works great is a lot faster ... 👍

@karlitschek
Copy link
Contributor

Two thumbs ups. Can we merge this? @PVince81

@PVince81
Copy link
Contributor Author

I'd like @schiesbn to have a quick look to make sure the code (especially PHP) is ok.

@schiessle
Copy link
Contributor

looks good!

schiessle added a commit that referenced this pull request Nov 25, 2013
Deleting all files in trash now only sends a single flag
@schiessle schiessle merged commit 31d0ba0 into master Nov 25, 2013
@schiessle schiessle deleted the filestrash-emptytrash branch November 25, 2013 13:24
@lock lock bot locked as resolved and limited conversation to collaborators Aug 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants